feat: add transaction filtering #272
Conversation
dimazhornyk
requested review from
vladbochok and
StanislavBreadless
as code owners
l1-contracts/contracts/state-transition/chain-interfaces/ITransactionFilterer.sol
Outdated
Show resolved
Hide resolved
| @@ -226,6 +227,10 @@ contract MailboxFacet is ZkSyncStateTransitionBase, IMailbox { | |||
| function _requestL2TransactionSender( | |||
| BridgehubL2TransactionRequest memory _request | |||
| ) internal nonReentrant returns (bytes32 canonicalTxHash) { | |||
| // Check that the transaction is allowed by the filterer (if the filterer is set). | |||
| if (s.transactionFilterer != address(0)) { | |||
| require(ITransactionFilterer(s.transactionFilterer).isTransactionAllowed(_request), "tf"); | |||
There was a problem hiding this comment.
Why do you need to submit the whole Transaction request for a check? This is gas-consuming, please only submit fields that makes sense to check (from?)
There was a problem hiding this comment.
My initial thought was to give as much flexibility as possible to the partners implementing the filters, but your point is valid so I reduced it to address sender, address contractL2, uint256 mintValue, uint256 l2Value
There was a problem hiding this comment.
By the Stas comments, you may consider adding calldata selector this list
There was a problem hiding this comment.
I meant selector (first 4 bytes of the calldata), not the whole calldata. This is to minimize the cost too
@StanislavBreadless wdyt?
There was a problem hiding this comment.
how is the selector going to be useful here? I thought that it would be more important to have the actual values of the calldata
There was a problem hiding this comment.
We talked with @vladbochok and it makes sense to pass the entire calldata
| @@ -226,6 +227,15 @@ contract MailboxFacet is ZkSyncStateTransitionBase, IMailbox { | |||
| function _requestL2TransactionSender( | |||
| BridgehubL2TransactionRequest memory _request | |||
| ) internal nonReentrant returns (bytes32 canonicalTxHash) { | |||
| // Check that the transaction is allowed by the filterer (if the filterer is set). | |||
| if (s.transactionFilterer != address(0)) { | |||
| require(ITransactionFilterer(s.transactionFilterer).isTransactionAllowed( | |||
There was a problem hiding this comment.
do you think it may make sense to require the entire BridgehubL2TransactionRequest? I am not sure about the impact on the cost of the transaction, but intuitively we might want to prevent users asking us for any additional parameters in the future for the tx validation
There was a problem hiding this comment.
For instance, access to calldata may be the only option to prevent malicious contracts from bridging funds (as such txs would have L1SharedBridge as the sender)
There was a problem hiding this comment.
added a calldata to the list of params, but not the full request to save some gas
l1-contracts/contracts/state-transition/chain-interfaces/ITransactionFilterer.sol
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
lgtm
Waiting for the hyperbridge design (ie L2<->L2 transactions) to see how hyperchains can behave pre-emptively to the transaction filtering.
Ideally there would be a way to tell the user that its transaction is going to fail before it actually reach the smart contracts on L1.
What ❔
Introduces an opportunity to add an L1->L2 transaction filtering. The address of the filterer is being set through the Admin facet and then reused in Mailbox's
_requestL2TransactionSenderto ensure that the transaction can be sent.Why ❔
Some partners want to run hyperchains with KYC/whitelisting of the addresses.
Checklist